[wasm64] making JS bindings wasm64 aware by aardappel · Pull Request #12869 · emscripten-core/emscripten

aardappel · 2020-11-24T01:07:38Z

This is a large collection of fixes resulting from trying to run the tests under wasm64. It is not complete, i.e. does not pass all tests, and requires a specific local setup. It is unknown how much work is left to pass all of them, though this allows significant amount of code to work.

I am trying to land these changes, because, well, maintaining this large fork is getting onerous, and it would be beneficial for wasm64 related changes to be already present, even if not used.

This PR has the wasm64 tests OFF, meaning it is meant to be a NFC for wasm32, effectively.

Of course, this PR contains a lot of unrelated fixes that ideally could have been spread out over 10+ PRs, had I known what I was doing. I think it made sense to initially collect all these changes in one PR, since it was hard to tell what shape things would take, and in fact there have been large sweeping changes that I later undid, which we've now all been saved from.

The question remains, should this PR be split up, and if so, in how many PRs? I would request to not go too crazy on this, this will not be a neat PR no matter how you slice it. Then there's of course the issue that there may be dependencies.

Follow-up to the work in #12658 and #14613

sbc100

Nice!

sbc100 · 2020-11-24T17:59:15Z

-  POINTER_SIZE: 4,
-  QUANTUM_SIZE: 4,
+  POINTER_SIZE: getPointerSize(),
+  POINTER_TYPE: MEMORY64 ? 'i64' : 'i32',


get you just call getPointerType() here?

You mean pointerType() from parseTools.js ? Is that always available to this file? I am still unclear on how all these JS files fit together :)

My understanding is that all of these files (modules.py, parseTools.js, runtime.js at least) all loaded together when processing JS so they all shared a single namespace.

I tried, and I can't access variables from parseTools.js here, though I might be getting it wrong :)

sbc100

I don't fully grok the makeGetValue / makeSetValue stuff bit everything else looks good!

sbc100 · 2020-12-04T21:17:43Z

If you feel like splitting of the Runtime.POINTER_SIZE -> POINTER_SIZE change that could land first? (No worries if not).

sbc100 · 2020-12-04T21:18:17Z

Is this still a draft?

aardappel · 2020-12-07T22:57:55Z

No, I'd rather not split that off.. and yes, still a draft until I get further along I think?

This reflects changes in the function signature in https://reviews.llvm.org/D101985 and also uses `from64` function defined in emscripten-core#12869. This has not been tested against wasm64 yet and I'm uploading this mainly for discussions at this point. Also in the team chat of the tools team we discussed other alternatives to `from64` function, which can be done separately and not reflected here.

There are a lot of places where pointers get cast to int, and to prepare for further Memory64 changes, those are changed to long. I chose long since it is most similar to the existing int, instead of e.g intptr_t, since long is more universal (there are many cases where the actual data stored is not a pointer). Since under wasm32 int and long are equal, this should in theory be a NFC for wasm32, making it easier to review. It also allows #12869 to be smaller

aardappel · 2021-09-23T19:23:18Z

Most tests are passing, I think this is ready to start final review, and/or any discussion about splitting up (please read updated description at the top first) @azakai @sbc100

aardappel · 2021-09-23T20:52:10Z

The only tests failing are test-browser-chrome, which seems to complain about not being able to start a HTTP server, I am guessing that is a flake?

kripken

Nice progress!

Reading this, I had an idea. This PR does the best we can do currently in terms in JS code quality, but that still brings downsides - but we could avoid them by porting code from JS to C. This is actually what we're doing with the filesystem rewrite @ethanalee is working on now, which means that it might be best to wait for that to land. In fact once #15104 lands then we could say that MEMORY64 depends on WASMFS and then we'd be limited to stdout (for now) but we'd have less JS to need to worry about.

Similarly, perhaps we could all as a team make an effort to port other places with lots of from64 and other JS workarounds in this PR to C. That's generally the right direction anyhow, and the result would be more maintainable and optimizable, and would make this PR smaller and better. WDYT?

aardappel · 2021-09-23T21:44:09Z

@kripken the reason I am trying to land this is because it is an enormous amount of work maintaining and testing it, so the prospect of waiting on a PR that is going to make yet more significant changes to this PR does not sound great to me. In fact, I'd like to land this with as few changes as possible, and leave any cleanup to future PRs, of which there will be many anyway.

Most changes that make things worse are pretty isolated, like you said if we find a better way to do from64 it is easy to find the locations to fix.

This PR should have very little effect on wasm32 and makes things strictly better for wasm64.

The way BigInt was designed in JS makes it surprisingly hard to do this stuff non-intrusively.

kripken · 2021-09-23T23:50:40Z

@aardappel I see, makes sense.

I wouldn't oppose landing this soon, then. However, I think we should agree as a team on a plan for the code here, that ends up getting to a good place in the near future.

sbc100 · 2021-09-24T00:19:55Z

  // have __heap_base hardcoded into it - it receives it from JS as an extern
  // global, basically).
-  __heap_base: '{{{ HEAP_BASE }}}',
+  __heap_base: '{{{ to64(HEAP_BASE) }}}',


Why is this ok to remain as is and not use new WebAssembly.Global but the above ones are not?

This was already not using new WebAssembly.Global before my change.. I have no idea why.

sbc100 · 2021-09-24T01:04:22Z

  implemented in javascript.
  """
-  command = config.NODE_JS + [filename] + jsargs
+  command = config.NODE_JS + ['--experimental-wasm-bigint'] + [filename] + jsargs


Is this needed for something?

Yes, the JS tools themselves may now contain code that uses bigints?

Is that something you have added in this PR? It seems odd that that would be needed just to process the library js code.. can you point to an example?

I'd have to run 64-bit test for that again to uncover that. Can we leave it in? If it's just for the tools it doesn't hurt. Or if you insist I can put it in a comment so it is easier to re-apply when we return to fixing 64-bit issues.

sbc100 · 2021-09-24T01:07:14Z

                               '-Wno-format',
                               '-nostdlib',
                               compiler_rt,
+                               '-s', 'MEMORY64=' + str(settings.MEMORY64),


I don't think this will work. This gets run just once one first use so we don't want to depend on the settings use in that moment. If there are differences here then we would need a second set of json file and have them stored separately in the cache.

See generate_struct_info in emscripten.py.

Ok, I guess I am not clear on how to fix this.. maybe we can discuss that offline. Or at least maybe I can just put a FIXME in there for when testing with wasm64 resumes.. so far this has worked in allowing tests to pass.

The fix I think would be to have a different filename when running in 64bit mode. See:

emscripten/emscripten.py

Line 826 in bca56b3

generated_struct_info_name = 'generated_struct_info.json'

It won't work as it stands because if you have previously built the struct_info.json with -sMEMORY64 and then try to build something without that flag .. it would use the wasm64 version of the json file.. and things be broken in horrible ways, right?

Ok, so we need a generated_struct_info64.json ? I can look into how to set that up in a follow-up. I'll comment this out for now.

sbc100 · 2021-10-04T17:13:39Z

I mean you can put the quotes outside the `{{{ }}}` and just have `POINTER_TYPE` inside.. avoiding the backticks (I think).

…

On Mon, Oct 4, 2021 at 10:08 AM Wouter van Oortmerssen < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/runtime_safe_heap.js <#12869 (comment)> : > @@ -14,7 +14,7 @@ @param {number|boolean=} noSafe */ function setValue(ptr, value, type, noSafe) { type = type || 'i8'; - if (type.charAt(type.length-1) === '*') type = 'i32'; // pointers are 32-bit + if (type.charAt(type.length-1) === '*') type = {{{ `'${POINTER_TYPE}'` }}}; That would generate i32 and it needs to generate 'i32' — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12869 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD55ZN7ZXBWUFMAF3OIOP3UFHNPXANCNFSM4UAGXGBQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

aardappel · 2021-10-14T18:59:03Z

Ok, what I just pushed is what remains after rebasing on top of the 15 PRs that were split out.

For reference:

failfast flag: Add a failfast command-line option to runner.py #15111
command-line flags: [Memory64] new 64-bit related command-line flags #15218
core JS runtime: [Memory64] Core JS runtime changes for 64-bit #15219
embind / safe heap JS: [Memory64] embind / safe heap JS changes for 64-bit #15220
dylink JS: [Memory64] dylink JS changes for 64-bit #15221
pthread JS: [Memory64] pthread JS changes for 64-bit #15222
syscall JS: [Memory64] syscall JS changes for 64-bit #15223
wasi JS: [Memory64] wasi JS changes for 64-bit #15224
preamble JS: [Memory64] preamble JS changes for 64-bit #15225
embind C++: [Memory64] embind C++ changes for 64-bit #15226
sanitizer C++: [Memory64] sanitizer C++ changes for 64-bit #15227
sanitizer header: [Memory64] sanitizer header changes for 64-bit #15228
pthread libc: [Memory64] pthread libc changes for 64-bit #15229
network libc: [Memory64] network libc changes for 64-bit #15230
pthread rwlock libc: [Memory64] pthread rwlock libc changes for 64-bit #15231

There may be further things you want split out @sbc100 @kripken or is this good to land?

kripken

How about splitting out the strftime and webidl binder changes (so that if we end up bisecting it doesn't reach here)? Otherwise, I think this can stay in one PR.

kripken · 2021-10-14T19:50:46Z

+  else:
+    benchmarkers += [
+      # EmscriptenBenchmarker('Node.js', config.NODE_JS),
+    ]


Wasm64 does seem special enough to justify this, I think. And we do have a few other global params here like profiling etc.

kripken · 2021-10-14T19:52:08Z

+                               # separately in the cache.
+                               # Whereever generated_struct_info.json is generated, there now
+                               # needs to be a generated_struct_info64.json for MEMORY64 mode.
+                               # '-s', 'MEMORY64=' + str(settings.MEMORY64),


I think we'd need a separate cache dir for memory64 (like for LTO), and to pass this tool a flag in that case, but I'm not sure.

Yes, we discussed this should be fixed in a follow-up, see #15218 (and also elsewhere in this PR)

sbc100 · 2021-10-14T19:51:08Z

+		*l = snprintf(*s, sizeof *s, "%+.2ld%.2ld", // XXX EMSCRIPTEN: %d => %ld
 			(tm->__tm_gmtoff)/3600,
-			abs(tm->__tm_gmtoff%3600)/60);
+			labs(tm->__tm_gmtoff%3600)/60); // XXX EMSCRIPTEN: abs => labs


Maybe better to disable that warning rather than hack musl like this. See:

emscripten/tools/system_libs.py

Lines 682 to 688 in dfb2d09

# Disable certain warnings for code patterns that are contained in upstream musl

cflags += ['-Wno-ignored-attributes',

'-Wno-dangling-else',

'-Wno-unknown-pragmas',

'-Wno-shift-op-parentheses',

'-Wno-string-plus-int',

'-Wno-pointer-sign']

Also, it looks like this was removed upstream so this issue will go away once I get around to landing #13006: https://github.com/emscripten-core/musl/

sbc100 · 2021-10-14T19:52:11Z

  set_env('EMTEST_REBASELINE', options.rebaseline)
  set_env('EMTEST_VERBOSE', options.verbose)
  set_env('EMTEST_CORES', options.cores)
+  set_env('EMTEST_FORCE64', options.force64)


This still needs to be resolved.. we shouldn't need the global keyword above.

sbc100 · 2021-10-14T19:52:47Z

+  else:
+    benchmarkers += [
+      # EmscriptenBenchmarker('Node.js', config.NODE_JS),
+    ]


@kripken WDYT about these changes to test_benchmark.py

kripken · 2021-10-14T21:22:51Z

@sbc100 benchmark changes look good to me. They aren't temporary changes, but a new mode for the runner. Makes sense to commit them I think.

aardappel · 2021-10-14T21:49:05Z

Two more:

webidl_binder: [Memory64] webidl_binder changes for 64-bit #15302
strftime: [Memory64] strftime changes for 64-bit #15303

kripken

Ok, if you'd rather do the getPointerSize() etc. stuff in a followup, that seems ok to me.

kripken · 2021-10-15T17:30:44Z

+  else:
+    benchmarkers += [
+      # EmscriptenBenchmarker('Node.js', config.NODE_JS),
+    ]


+1 from me for these changes to benchmarking.

This regressed in #12869 - while adding wasm64 support we reordered how the args are added. The extra emcc args must be at the end so that they apply on top of the other ones, in particular, some tests must disable minimal runtime.

This regressed in emscripten-core#12869 - while adding wasm64 support we reordered how the args are added. The extra emcc args must be at the end so that they apply on top of the other ones, in particular, some tests must disable minimal runtime.

sbc100 reviewed Nov 24, 2020

View reviewed changes

vadimkantorov mentioned this pull request Nov 25, 2020

Can't build GNU diffutils with emscripten #12879

Open

aardappel marked this pull request as draft December 4, 2020 00:22

sbc100 reviewed Dec 4, 2020

View reviewed changes

Comment thread system/lib/pthread/library_pthread_stub.c Outdated

Comment thread src/preamble.js

Comment thread src/preamble.js Outdated

Base automatically changed from master to main March 8, 2021 23:49

aheejin mentioned this pull request May 6, 2021

Support Emscripten EH/SjLj in Wasm64 #14108

Closed

sbc100 mentioned this pull request Jul 2, 2021

Building and Running WASM in -sMEMORY64=1 mode #14582

Closed

aardappel mentioned this pull request Jul 9, 2021

[Memory64] Replace int (used as ptr) with long in tests #14613

Merged

aardappel marked this pull request as ready for review September 23, 2021 19:21

kripken reviewed Sep 23, 2021

View reviewed changes

Comment thread emscripten.py Outdated

Comment thread src/parseTools.js

Comment thread system/lib/libc/musl/src/network/recvmsg.c Outdated

Comment thread tests/runner.py

sbc100 reviewed Sep 24, 2021

View reviewed changes

sbc100 mentioned this pull request Sep 27, 2021

Where possible, we should move JavaScript to native/wasm code #15151

Open

juj reviewed Oct 4, 2021

View reviewed changes

Comment thread src/parseTools.js

kripken reviewed Oct 4, 2021

View reviewed changes

This was referenced Oct 4, 2021

[Memory64] Core JS runtime changes for 64-bit #15219

Merged

[Memory64] new 64-bit related command-line flags #15218

Merged

kripken reviewed Oct 14, 2021

View reviewed changes

sbc100 reviewed Oct 14, 2021

View reviewed changes

[wasm64] making JS bindings wasm64 aware

32ca8dc

aardappel mentioned this pull request Oct 14, 2021

[Memory64] strftime changes for 64-bit #15303

Closed

kripken approved these changes Oct 15, 2021

View reviewed changes

aardappel merged commit 8d424d2 into emscripten-core:main Oct 15, 2021

kripken mentioned this pull request Nov 18, 2021

Fix benchmark suite #15566

Merged

	# Disable certain warnings for code patterns that are contained in upstream musl
	cflags += ['-Wno-ignored-attributes',
	'-Wno-dangling-else',
	'-Wno-unknown-pragmas',
	'-Wno-shift-op-parentheses',
	'-Wno-string-plus-int',
	'-Wno-pointer-sign']

Uh oh!

Conversation

aardappel commented Nov 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbc100 commented Dec 4, 2020

Uh oh!

sbc100 commented Dec 4, 2020

Uh oh!

aardappel commented Dec 7, 2020

Uh oh!

aardappel commented Sep 23, 2021

Uh oh!

aardappel commented Sep 23, 2021

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aardappel commented Sep 23, 2021

Uh oh!

kripken commented Sep 23, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 commented Oct 4, 2021 via email

aardappel commented Nov 24, 2020 •

edited

Loading